PMM-15360: Add the OpenManager settings flag - #5866
Conversation
68eea37 to
839e3dc
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## PMM-15299-open-manager #5866 +/- ##
==========================================================
- Coverage 47.07% 47.06% -0.01%
==========================================================
Files 430 430
Lines 45467 45481 +14
==========================================================
+ Hits 21402 21405 +3
- Misses 22004 22015 +11
Partials 2061 2061 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1de5337 to
be40598
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThe change adds OpenManager configuration to the v1 API and server settings model. It supports updates through Merge Risk: 🟡 Moderate · up to The OpenManager setting is exposed through the API and UI, but the current update path does not persist the toggle, so users may see a successful change without the server state changing. The persistence mapping should be fixed before merging; the remaining accessibility and URL cleanup are non-blocking. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Warning Some tools did not complete. Review the errors below. 🔧 Buf (1.72.0)api/server/v1/server.protofatal: unable to access 'https://github.com/percona/pmm.git/': Failed to connect to github.com port 443 via 127.0.0.1 after 0 ms: Could not connect to server Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ui/apps/pmm/src/pages/settings/Settings.messages.ts (1)
51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
openManagerLinkto the shared URL constants.This line hardcodes a URL in
Settings.messages.ts. Define the URL insrc/lib/constants.tsand reference that constant here.As per coding guidelines,
ui/apps/pmm/**/*.{js,jsx,ts,tsx}must not hardcode URLs; use constants fromsrc/lib/constants.ts.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: adfff729-cedd-4345-a7dd-1199eea48e13
⛔ Files ignored due to path filters (1)
api/server/v1/server.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
api/server/v1/json/client/server_service/change_settings_responses.goapi/server/v1/json/client/server_service/get_read_only_settings_responses.goapi/server/v1/json/client/server_service/get_settings_responses.goapi/server/v1/json/v1.jsonapi/server/v1/server.pb.validate.goapi/server/v1/server.protoapi/swagger/swagger-dev.jsonapi/swagger/swagger.jsonmanaged/models/settings.gomanaged/models/settings_helpers.gomanaged/models/settings_helpers_test.gomanaged/services/server/server.gomanaged/utils/envvars/parser.goui/apps/pmm/src/pages/settings/Settings.messages.tsui/apps/pmm/src/pages/settings/components/advanced/AdvancedSettingsForm.schema.tsui/apps/pmm/src/pages/settings/components/advanced/AdvancedSettingsForm.tsxui/apps/pmm/src/pages/settings/components/advanced/AdvancedSettingsForm.utils.tsui/apps/pmm/src/types/settings.types.tsui/apps/pmm/src/utils/testUtils.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.
be40598 to
04f695f
Compare
| accessControlTooltip: | ||
| 'Restrict data visibility based on user roles and labels.', | ||
| accessControlLink: 'https://per.co.na/roles_permissions', | ||
| openManagerLabel: 'OpenManager', |
There was a problem hiding this comment.
@catalinaadam FIY, since this will be visible on PMM page
|
@plebioda just noticed that the UI build here is failing do a missing property in type (https://github.com/percona/pmm/actions/runs/33516832877/job/99885859275?pr=5866#step:8:46) |
|
CI failure fixed in 4392e19: the main merge added a hand-written literal settings object for anonymous users in |
Formal ancestry only -- om-nav-gate's own PMM-15360 Fix two type errors the merge with main surfaced (c17476c) already carries the identical omEnabled fix, so this merge changes no files. Brings the integration branch's switch-flag lineage back in sync with the fix pushed directly to PMM-15360-om-switch-flag after CI caught it on #5866.
Adds Settings.OpenManager.Enabled to pmm-managed and an Advanced Settings toggle for it, default off like every other technical-preview flag (Azure Discover, Access Control). PMM_ENABLE_OM seeds it the same way PMM_ENABLE_NOMAD/PMM_ENABLE_AZURE_DISCOVER do, and reuses validateChangeSettingsRequest's existing env-var-wins precedence check rather than adding a new mechanism. This only exposes and stores the flag; nothing reads it yet to gate OpenManager's own behavior. That's the follow-up PR, once this and PMM-15326-om-backend / PMM-15326-om-ui-nav are all available to stack on. Signed-off-by: Pawel Lebioda <pawel.lebioda@percona.com>
4392e19 to
3ae169d
Compare
…abled make gen does more than buf generate -- it also regenerates the swagger/OpenAPI JSON (api/server/v1/json/v1.json, the merged api/swagger/swagger*.json) and the generated JSON API client (api/server/v1/json/client/server_service/*.go) from the same proto. The earlier commit only ran buf generate directly and hand-curated its output against unrelated toolchain drift elsewhere in api/, which missed these -- they're make gen's job, not buf generate's. Also picks up the same protoc-gen-go import-grouping drift on server.pb.go/server.pb.validate.go that affects unrelated proto packages across the tree; taking make gen's own output here (rather my earlier hand-curated one) is what actually matches what CI's own gen step produces, so this and CI won't disagree again. The generated client's field names are OmEnabled/EnableOm here, matching this branch's own api/Makefile: --additional-initialism=om was added to the swagger client generator's flag list by PMM-15326-om-api, but that commit hasn't reached this branch's base (PMM-15299-open-manager) yet, so "om" isn't a registered initialism here and go-swagger capitalizes it as an ordinary word. Branches that already have that flag (PMM-15326-om-backend and its descendants) will correctly want OMEnabled/EnableOM instead -- each PR's generated code has to match its own base's Makefile, not a single "right" answer across the stack. Found because CI's "make gen; git diff --exit-code" check failed on this exact mismatch, on all three of this ticket's PRs. Signed-off-by: Pawel Lebioda <pawel.lebioda@percona.com>
The main merge added a hand-written literal settings object for anonymous users in SettingsProvider but never included the omEnabled field this branch adds to ReadonlySettings, so tsc failed the build with 'Property omEnabled is missing'. Signed-off-by: Pawel Lebioda <pawel.lebioda@percona.com>
3ae169d to
72b8283
Compare
3 participants
Recreated after #5851 was merged by mistake and reverted on
PMM-15299-open-manager(see #5851 for the original description and discussion).Same two commits as the original PR, unchanged: